-
Notifications
You must be signed in to change notification settings - Fork 23.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PSYNC2: make partial sync possible after master reboot #8015
PSYNC2: make partial sync possible after master reboot #8015
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soloestoy i think we better have a test for that.
@guybe7 please have a look.
(adding major decision due to the new info field)
ac02828
to
9e82c2f
Compare
I think this PR tries to solve partial sync when master reboots before replicas do failover. |
Hi @oranagra @soloestoy I think that's a great idea, especially, we have changed 'random sampling garbage collector' to 'consecutive garbage collector' in I think what oran said makes sense, maybe we can refactor master behavior. I notice we don't create master /* If no expired key is deleted when load RDB, we may have chance to
* restore replication context to implement partial resynchronizations. */
if (!server.rdb_expired_keys_last_load) {
memcpy(server.replid,rsi.repl_id,sizeof(server.replid));
server.master_repl_offset = rsi.repl_offset;
if (!iAmMaster()) {
/* If this is a replica, create a cached master from this
* information, in order to allow partial resynchronizations
* with masters, like REPLICAOF ip port. */
replicationCacheMasterUsingMyself();
selectDb(server.cached_master,rsi.repl_stream_db);
} else {
/* If this is a master, shift the current replication ID and
* offset as secondary replication ID to allow replicas to partial
* resynchronizations with masters, like REPLICAOF NO ONE. */
shiftReplicationId();
createReplicationBacklog();
server.repl_no_slaves_since = time(NULL);
}
} Just FYI, no test :) |
To avoid expired keys to make replication context invalid, could we add a config |
@soloestoy seems we have forgotten this PR although i don't see any major issues that were discussed. let's pick it up again and finish it for 7.0? Regarding the last comment by @ShooterIT, i don't like to add that config, but maybe we can write to the replication backlog when we're expiring keys during rdb load, this way if there weren't too many expired keys, replicas will still be able to psync. |
@oranagra I will rebase this PR.
I like this approach. |
@oranagra Sadly, this cannot work, because If we can make |
the server just restarted, if there are any slaves already connected, they won't be in an ONLINE state yet. |
9e82c2f
to
8b5bab2
Compare
@oranagra I mean the Time 1. master offset is 100 without PING and generate an RDB with offset 100 |
@soloestoy that's right, but what's the difference between that case and the case in which we don't DEL commands on behalf of expired keys? i.e. a master restarts from rdb file, then receives a bunch of commands from clients before the replica attempts to PSYNC. besides, if i understand correctly, the main issue that this PR comes to solve is about a graceful restart of the master (one that saves an rdb file on shutdown), so in that case the problem you described doesn't exist. p.s. another improvement to that problem, and possibly to other problems is that when the server goes down gracefully, it stores the replication backlog into the rdb file (and both replid1 and replid2 and their offsets). |
@oranagra thanks, I got it. |
If both AOF and RDB are enabled and a replica - or master in the case of new implementation - restarts, should it ignore replication metadata because it loads AOF and not RDB? That's what I've noticed. Maybe a separated issue? |
@eduardobr we never support partial resync after restart from AOF, only restart from RDB have the chance. |
@soloestoy Do you know if there are limitations that require us to store metadata in the .rdb only? |
Maybe when redis saves an RDB file during shutdown it should delete the AOF? and when it starts, copy the RDB file to an AOF (serving as preamble AOF). |
The main idea is how to allow a master to load replication info from RDB file when rebooting, if master can load replication info it means that replicas may have the chance to psync with master, it can save much traffic. The key point is we need guarantee safety and consistency, so there are two differences between master and replica: 1. master would load the replication info as secondary ID and offset, in case other masters have the same replid. 2. when master loading RDB, it would propagate expired keys as DEL command to replication backlog, then replica can receive these commands to delete stale keys. p.s. the expired keys when RDB loading is userful for users, so we show it as rdb_expired_keys_last_load in info persistence. Moreover, after load replication info, master should update no_replica_time in case loading RDB cost too long time.
8b5bab2
to
1a39dc0
Compare
PR updated, please check @oranagra |
I don't think it's a good time to do it, since the multi-part AOF may change a lot. |
The idea sounds good. But it raises another question, if we use RDB preambles in our AOF and RDB is also enabled then each RDB saving can be treated as an AOF rewrite. The RDB saving during shutdown is just a simple case of an AOF rewrite without buffering. |
I discussed it with Yossi, and realized that just deleting the AOF on shutdown is not good since on startup, if we start without an AOF file, we'll have to do an initial rewrite or rename the RDB file, so that we can accumulate writes into it. bottom line, i agree we want to merge this PR as is, and re-open this discussion after multi-part AOF is implemented (or when working on it).
|
Maybe just an easy way can work: when |
I don't like it. I'm mainly aiming for restart after a graceful shutdown, in that case we can store some metadata in the tail of the aof, but I'd also like to avoid loading a huge aof file on startup. Anyway, since there's no trivial solution I agree we should revisit that after the multipart aof change. |
^ 👍 |
1fd5545
to
c83d5a7
Compare
Related #9513. |
This PR aims to solve issue #6030, about making psync possible after master reboot, at first I wrote some codes in #6034 but it's not good enough, after a discussion with @oranagra , I modify the code(about the expire problem) and open the new one, here ping @redis/core-team I'd like to have your suggestions.
The main idea is how to allow a master to load replication info from RDB file when rebooting, if master can load replication info it means that replicas may have the chance to psync with master, it can save much traffic.
The key point is we need guarantee safety and consistency, so there
are two differences between master and replica:
offset, in case other masters have the same replid.
command to replication backlog, then replica can receive these
commands to delete stale keys.
p.s. the expired keys when RDB loading is useful for users, so
we show it as
rdb_last_load_keys_expired
andrdb_last_load_keys_loaded
in info persistence.Moreover, after load replication info, master should update
no_replica_time
in case loading RDB cost too long time.